Fix routing YAML best results export#1422
Open
fallintoplace wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR fixes best result serialization logic in ChangesBest results serialization and validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuopt/cuopt/tests/routing/test_solver_settings.py`:
- Around line 56-58: Tests use hardcoded filenames (config_file =
"solver_cfg.yaml", best_results_file = "best_results.txt") when calling
s.dump_config_file and s.dump_best_results which can collide in parallel runs;
change to use the pytest tmp_path fixture to create per-test files/paths (e.g.,
tmp_path / "solver_cfg.yaml" and tmp_path / "best_results.txt") and pass those
Path strings to s.dump_config_file and s.dump_best_results, and apply the same
tmp_path-based replacements for the other occurrences referenced around lines
72-75 so each test writes to isolated temporary files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aad36dbe-fcf2-4f80-ba50-bb871cf12d09
📒 Files selected for processing (2)
python/cuopt/cuopt/routing/utils.pypython/cuopt/cuopt/tests/routing/test_solver_settings.py
Comment on lines
+56
to
+58
| best_results_file = "best_results.txt" | ||
| s.dump_config_file(config_file) | ||
| s.dump_best_results(best_results_file, 0) |
There was a problem hiding this comment.
Use per-test temporary paths to avoid filename collisions.
Hardcoded solver_cfg.yaml / best_results.txt can cause flaky behavior in parallel or repeated test runs. Use tmp_path-scoped files.
Suggested change
-def test_dump_config():
+def test_dump_config(tmp_path):
"""Test SolverSettings solve with config file"""
s = routing.SolverSettings()
- config_file = "solver_cfg.yaml"
- best_results_file = "best_results.txt"
+ config_file = tmp_path / "solver_cfg.yaml"
+ best_results_file = tmp_path / "best_results.txt"
s.dump_config_file(config_file)
s.dump_best_results(best_results_file, 0)
assert s.get_config_file_name() == config_file
@@
- with open(config_file) as f:
+ with open(config_file) as f:
config = yaml.safe_load(f)
- assert config["best_result_path"] == best_results_file
+ assert config["best_result_path"] == str(best_results_file)
assert config["best_result_interval"] == 0As per coding guidelines, **/*test*.{cpp,py,cu,cuh}: “Prevent flaky tests from GPU timing, uninitialized memory, or race conditions.”
Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cuopt/cuopt/tests/routing/test_solver_settings.py` around lines 56 -
58, Tests use hardcoded filenames (config_file = "solver_cfg.yaml",
best_results_file = "best_results.txt") when calling s.dump_config_file and
s.dump_best_results which can collide in parallel runs; change to use the pytest
tmp_path fixture to create per-test files/paths (e.g., tmp_path /
"solver_cfg.yaml" and tmp_path / "best_results.txt") and pass those Path strings
to s.dump_config_file and s.dump_best_results, and apply the same tmp_path-based
replacements for the other occurrences referenced around lines 72-75 so each
test writes to isolated temporary files.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Routing YAML export used
dict.updatewith a set when best-results settings were enabled. That raises at runtime instead of writingbest_result_pathandbest_result_interval.This writes those fields with normal key assignment and treats
Noneas the only missing interval value, so an explicit interval of0is preserved.Validation
python3 -m py_compile python/cuopt/cuopt/routing/utils.py python/cuopt/cuopt/tests/routing/test_solver_settings.pygit diff --checkNot run locally:
PYTHONPATH=python/cuopt uvx --with 'pytest<9.0' --with pyyaml --with numpy pytest python/cuopt/cuopt/tests/routing/test_solver_settings.py -k 'dump_config' -qcould not collect becausecudfis not installed in this local environment.